fix(triggers): surface listener panics as errors instead of dropping events (EN-1222) - #185
fix(triggers): surface listener panics as errors instead of dropping events (EN-1222)#185flemzord wants to merge 1 commit into
Conversation
…events handleMessage had an unnamed error return, so its recover() returned nil and watermill ACKed the message. getWorkflowIDFromEvent deliberately panicked on (un)marshal failures (e.g. a SAVED_PAYMENT payload whose id is not a string), so a malformed event was permanently dropped with only a stdout print. - getWorkflowIDFromEvent now returns an error instead of panicking. - handleMessage uses a named return so a recovered panic is converted into a returned error (NACK + redelivery/DLQ) and logged with its stack via the structured logger. Adds a test asserting a malformed payment payload returns an error. Note: overlaps internal/triggers/listener.go with #184 (idempotence); the two are independent and may need a trivial merge.
|
Warning Review limit reached
More reviews will be available in 27 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
flemzord
changed the title
|
Superseded by #199, which consolidates this change with the related reliability and safety fixes on top of the current main branch. |
1 participant
Problem (H2 — HIGH)
handleMessagehad an unnamed error return:Recovering a panic therefore left the return value
nil, so watermill ACKed the message. AndgetWorkflowIDFromEventdeliberatelypanic()s on (un)marshal failures (e.g. aSAVED_PAYMENTpayload whoseidis not a string). Net effect: a single malformed event is permanently dropped, with only a stdout print — no metric, no redelivery.Fix
getWorkflowIDFromEventreturns anerrorinstead of panicking;handleMessagewraps and returns it.handleMessageuses a named return so a recovered panic is converted into a returned error (NACK → redelivery / DLQ) and logged with its stack via the structured logger.Test
TestHandleMessage/malformed payment payload returns an error instead of being dropped— aSAVED_PAYMENTwith a numericidnow returns an error.Severity: HIGH.